-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: symmetric key notifications #169
Conversation
Implements public announcements encrypted via symmetric keys This is most useful for utxos that transfer funds within a single wallet such as change addresses. compared to on-chain pub-key public announcements: Data encrypted with symmetric keys is smaller than data encrypted with asymmetric keys so there is a blockchain space (and thus fee) savings. compared to off-chain expected utxos: symmetric-key announcements exist on the blockchain and thus are immune to local data-loss situations. off-chain expected utxos do not use any blockchain space but require that the wallet holder make ongoing backups of wallet state and never lose them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By and large: good stuff, kudos!
Some requests:
- Please add a page to the documentation covering symmetric keys for change UTXOs.
- Avoid importing tools from
generation_address
insymmetric_key
. Instead, factor them out to a helper module. UtxoNotifierMethodSpecifier
andUtxoNotifierMethod
seem similar and I wonder if they cannot be merged.- Consider renaming
SpendingKeyType
toSpendingKey
andgeneration_address::SpendingKey
togeneration_address::GenerationSpendingKey
. - Please convert the randomized tests in
symmetric_key.rs
into proptests. Have a look inprimitive_witness.rs
if you need something to pattern match on. (I realize there are plenty of tests out there that are randomized withthread_rng
but I should like to convert those to proptests at some point. Let's not add to that conversion workload.) - Please add an integration test (not sure that's the right word) whereby you spawn two global states, one of which is a premine recipient whose sends some money to the other node and gets some change, and verify that the change is being observed and recovered correctly, even if the node in question reboots and loses everything but the seed phrase.
} | ||
} | ||
|
||
/// encodes this address as bech32m | ||
pub fn to_bech32m(&self, network: Network) -> Result<String> { | ||
match self { | ||
Self::Generation(k) => k.to_bech32m(network), | ||
Self::Symmetric(_k) => unimplemented!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a text string explaining why it doesn't make any sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why we need bech32m encoding (or something similar) for non-change payment addresses is because those addresses are transmitted off-band and need to be encoded for that purpose, preferably in a human-verifiable way. It doesn't make any sense for change "addresses" because there is no need for transmission from one party to another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep yep. I will add a comment.
Err(_) => false, | ||
} | ||
|
||
// ... that are marked as symmetric key encrypted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ... that are marked as symmetric key encrypted | |
// ... that are marked as generation address ciphertext |
generation_address::shake256::<32>( | ||
&bincode::serialize(&self.seed).expect("serialization should always succeed"), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the shake256
convenience wrapper is useful, it should probably be defined elsewhere. Probably in twenty-first
. Importing it from generation_address
is bad form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does exist in twenty-first, but is private for some reason. There is actually a comment about that in generation_address::shake256, that it was copied from twenty-first for that reason. I can make it pub in twenty-first, but that change might take a while to make it into neptune-core, so for now I guess I will put it into a shared module, with a comment to use twenty-first version when available.
|
||
/// returns the receiver_identifier, a public fingerprint | ||
pub fn receiver_identifier(&self) -> BFieldElement { | ||
generation_address::derive_receiver_id(self.seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to import things from generation_address
. What if we should decide to get rid of that? IMO, symmetric_key
should stand on its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I can refactor common code out into a shared module.
#[test] | ||
fn test_encrypt_decrypt() { | ||
let mut rng = thread_rng(); | ||
|
||
// 1. generate key from random seed. | ||
let symmetric_key = SymmetricKey::from_seed(rng.gen()); | ||
|
||
// 2. create utxo with random amount | ||
let amount = NeptuneCoins::new(rng.gen_range(0..42000000)); | ||
let utxo = Utxo::new_native_coin(symmetric_key.lock_script(), amount); | ||
|
||
// 3. generate sender randomness | ||
let sender_randomness: Digest = rng.gen(); | ||
|
||
// 4. encrypt secrets (utxo, sender_randomness) | ||
let ciphertext = symmetric_key.encrypt(&utxo, sender_randomness).unwrap(); | ||
println!("ciphertext.get_size() = {}", ciphertext.len() * 8); | ||
|
||
// 5. decrypt secrets | ||
let (utxo_again, sender_randomness_again) = symmetric_key.decrypt(&ciphertext).unwrap(); | ||
|
||
// 6. verify that decrypted secrets match original secrets | ||
assert_eq!(utxo, utxo_again); | ||
assert_eq!(sender_randomness, sender_randomness_again); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider converting this test to a proptest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. It seems it would mainly test the (3rd party) implementation of Aes256. Is that the intent?
closing in favor of #171. |
closes #161.
Implements public announcements encrypted via symmetric keys
This is most useful for utxos that transfer funds within a single wallet such as change addresses.
Compared to on-chain pub-key public announcements:
Data encrypted with symmetric keys is smaller than data encrypted with asymmetric keys so there is a blockchain space (and thus fee) savings.
Compared to off-chain expected utxos:
symmetric-key announcements exist on the blockchain and thus are immune to local data-loss situations. off-chain expected utxos do not use any blockchain space but require that the wallet holder make ongoing backups of wallet state and never lose them.
Design choices, please review:
SymmetricKey
presently usesAes256Gcm
. Is this our final choice?Symmetric keys are derived from a seed and are unique for each utxo. (rather than using a single sym-key per wallet shared for all utxo.) This is because we want the receiver_id to be unique to avoid linking utxos.
Symmetric keys derive from the same root seed
WalletSecret::secret_seed
as generation addresses but the derivation in::nth_symmetric_key()
uses aSYMMETRIC_KEY_FLAG
. This means that end-user should only require a single mnemonic phrase for the root seed. A symmetric-key derivation counter must be stored in wallet state.efficiency:
SymmetricKey
presently derives all fields from seed as needed, rather than pre-calc and store when created. see doc-comment for the struct.change notify method now defaults to on-chain symmetric key, rather than offchain (expected utxo) in dashboard and neptune-cli. (note: this could easily be made an end-user option)
Summary of changes:
Cargo.toml:
std
so that the aead::Error impls std::error::Error trait.New code:
AnnouncedUtxo
to represent a found/claimed utxo and its secrets.UtxoNotifyMethodSpecifier
that further simplifies create_transaction() params and impl.SymmetricKey
WalletSecret::next_unused_symmetric_key()
and::nth_symmetric_key()
WalletState::get_known_symmetric_keys()
Modified code:
SymmetricKey
toSpendingKeyType
andReceivingAddressType
.UtxoNotificationPool::scan_for_expected_utxos()
WalletState::scan_for_announced_utxos()
to include symmetric keys.WalletState::find_spending_key_for_utxo()
to include symmetric keys.WalletState::update_wallet_state_with_new_block()
GlobalState::create_transaction()
. simplify args and impl.rpc_server::send()
andsend_to_many()
to acceptchange_utxo_notify_method
param.OffChainSymmetricKey
flag forsend()
parameterchange_notify_method
.New Tests:
symmetric_key::test::test_encrypt_decrypt()
symmetric_key::test::scan_for_announced_utxos_test()